feat: add support for composite types in go-tools#1448
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the gRPC compiler to use descriptor-driven field assignment, adds modular per-field handlers (oneofs, repeated, message, enum), changes null/presence semantics, threads GRPCMapping into the required-fields visitor with inline-fragment tracking, and expands federation tests, mappings, mock RPCs, and proto types for abstract/oneof scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Compiler
participant Dispatcher
participant Handlers
participant Proto
Client->>Compiler: provide JSON input + RPCMessage template
Compiler->>Dispatcher: iterate RPC fields -> processRPCField(field, fd, data)
alt oneof field
Dispatcher->>Handlers: buildOneOfMessage(__typename, fragmentFields)
Handlers->>Proto: select variant descriptor and populate sub-message
else repeated field
Dispatcher->>Handlers: processRepeatedField(elements, fd)
Handlers->>Handlers: build each element (may recurse)
Handlers->>Proto: append element to repeated field
else message field
Dispatcher->>Handlers: processMessageField(data, fd)
Handlers->>Handlers: resolveNestedMessage / wrapper handling
Handlers->>Proto: set nested message
else enum field
Dispatcher->>Handlers: processEnumField(name, fd)
Handlers->>Proto: set numeric enum value
else scalar field
Dispatcher->>Proto: set scalar value using fd
end
Compiler->>Compiler: isNullValue checks enforce optional/required semantics
Compiler->>Client: return populated protobuf message
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…n requiredFieldsVisitor
… ludwig/eng-9219-add-support-for-abstract-types-in-go-tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.go (1)
110-125:⚠️ Potential issue | 🔴 CriticalReuse the existing parent field before descending into fragment-only sub-selections.
If the same object field is selected once at the interface level and then refined inside an inline fragment, the duplicate check skips creating the fragment-local parent field,
enterNestedFieldreturnsfalse, and the walker still records the fragment’s grandchildren on the enclosing composite message. For example,pet { owner { id } ... on Cat { owner { age } } }will shape theCatfragment incorrectly becauseageis no longer attached underowner.Message. This needs to merge/reuse the existingownerfield before descent, or stop traversing that subtree when no target child message was found.Based on learnings: for composite messages in this package, interface-level fields in
message.Fieldsand type-specific inline-fragment fields both need to be preserved correctly.Also applies to: 129-142, 163-200
🧹 Nitpick comments (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go (1)
1807-1819: Add one direct-interface@requirescase.These new cases only exercise fragment-local selections, so a regression that drops shared interface fields like
nameor__typenamefrom the composite request would still pass. Please add one case likeprimaryItem { name ... on PalletItem { palletCount } ... on ContainerItem { containerSize } }so the planner path for common interface fields is covered too.Based on learnings: composite
@requireshandling carries common interface fields through fragment-based selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go` around lines 1807 - 1819, Add a test case that exercises a direct-interface `@requires` path which preserves common interface fields in fragment-local selections: in execution_plan_requires_test.go add a new case alongside "requires with interface type in selection set" that uses the same query structure but sets the federationConfigs SelectionSet for the Storage.itemInfo/primaryItem to "primaryItem { name ... on PalletItem { palletCount } ... on ContainerItem { containerSize } }" (or the equivalent selection used in the comment) so the planner is forced to carry through the shared interface field "name" and __typename across the composite request; update the test vector (the case name and mapping/testMapping()) accordingly so this new case runs with the existing test harness.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go (1)
1724-1810: Please add one nested abstract-path datasource case.The planner suite now covers nested shapes like
securitySummary,itemHandlerInfo,itemSpecsInfo, anddeepItemInfo, but this end-to-end suite only runs the flatitemInfo/operationReportpaths. A single nested case here would protect the compiler + JSON execution path that is most likely to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go` around lines 1724 - 1810, Add one nested abstract-path test case to the existing testCases slice: create a case named like "Query Storage with deepItemInfo nested abstract path" that queries _entities for Storage returning id, name and a nested abstract field (use deepItemInfo or securitySummary/itemHandlerInfo as the nested path) and supply vars with representations that include the nested __typename structures (e.g., Storage -> deepItemInfo -> primaryItem with PalletItem/ContainerItem). In federationConfigs add the base Storage selection (id) and a FieldName entry for the nested abstract field with a SelectionSet that drills into the nested object and includes inline fragments for concrete types (e.g., primaryItem { ... on PalletItem { name palletCount } ... on ContainerItem { name containerSize } }). Implement validate to assert _entities length, Storage id/name and that the nested field string is formatted as expected, and set validateError to require.Empty; reuse existing pattern from the "itemInfo" and "operationReport" cases so test structure and names (testCases, federationConfigs, validate, validateError) match the surrounding tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.go`:
- Around line 1807-1819: Add a test case that exercises a direct-interface
`@requires` path which preserves common interface fields in fragment-local
selections: in execution_plan_requires_test.go add a new case alongside
"requires with interface type in selection set" that uses the same query
structure but sets the federationConfigs SelectionSet for the
Storage.itemInfo/primaryItem to "primaryItem { name ... on PalletItem {
palletCount } ... on ContainerItem { containerSize } }" (or the equivalent
selection used in the comment) so the planner is forced to carry through the
shared interface field "name" and __typename across the composite request;
update the test vector (the case name and mapping/testMapping()) accordingly so
this new case runs with the existing test harness.
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.go`:
- Around line 1724-1810: Add one nested abstract-path test case to the existing
testCases slice: create a case named like "Query Storage with deepItemInfo
nested abstract path" that queries _entities for Storage returning id, name and
a nested abstract field (use deepItemInfo or securitySummary/itemHandlerInfo as
the nested path) and supply vars with representations that include the nested
__typename structures (e.g., Storage -> deepItemInfo -> primaryItem with
PalletItem/ContainerItem). In federationConfigs add the base Storage selection
(id) and a FieldName entry for the nested abstract field with a SelectionSet
that drills into the nested object and includes inline fragments for concrete
types (e.g., primaryItem { ... on PalletItem { name palletCount } ... on
ContainerItem { name containerSize } }). Implement validate to assert _entities
length, Storage id/name and that the nested field string is formatted as
expected, and set validateError to require.Empty; reuse existing pattern from
the "itemInfo" and "operationReport" cases so test structure and names
(testCases, federationConfigs, validate, validateError) match the surrounding
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c60c436e-f4cc-4659-bc5e-69ebd96ac345
⛔ Files ignored due to path filters (2)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.gov2/pkg/grpctest/productv1/product_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
v2/pkg/engine/datasource/grpc_datasource/compiler.gov2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.gov2/pkg/engine/datasource/grpc_datasource/compiler_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_requires_test.gov2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_federation_test.gov2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.gov2/pkg/engine/datasource/grpc_datasource/required_fields_visitor.gov2/pkg/engine/datasource/grpc_datasource/required_fields_visitor_test.gov2/pkg/grpctest/mapping/mapping.gov2/pkg/grpctest/mockservice_requires.gov2/pkg/grpctest/product.protov2/pkg/grpctest/testdata/products.graphqls
💤 Files with no reviewable changes (1)
- v2/pkg/engine/datasource/grpc_datasource/compiler_input_presence_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
1131-1147: Consider logging when oneof resolution falls back to empty message.When
FragmentFields[typeName]is not found (line 1139) orfindOneOfFieldDescriptorreturns nil (line 1145), the function returns an empty message without any indication. This could make debugging schema misconfigurations difficult in production.💡 Optional: Add debug logging for silent fallbacks
If debugging visibility is desired, consider adding optional logging when these fallbacks occur:
fragmentFields, ok := rpcMessage.FragmentFields[typeName] if !ok { + // Consider: log.Debug("no fragment fields found for typename", "typename", typeName) return message, nil } // Find the matching oneof field descriptor for the expected concrete type. oneofFieldDesc := p.findOneOfFieldDescriptor(inputMessage.Desc.Oneofs(), protoref.Name(rpcMessage.OneOfType.FieldName()), protoref.Name(typeName)) if oneofFieldDesc == nil { + // Consider: log.Debug("no oneof field descriptor found", "typename", typeName) return message, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/datasource/grpc_datasource/compiler.go` around lines 1131 - 1147, In buildOneOfMessage on RPCCompiler, add debug/error logging when the function currently silently returns an empty message: log when rpcMessage.FragmentFields[typeName] is missing (include typeName and rpcMessage identity) and when findOneOfFieldDescriptor(...) returns nil (include typeName and rpcMessage.OneOfType.FieldName()); ensure logs give enough context to trace schema/resolution issues (use existing logger/ctx in this package) before returning the empty message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/compiler.go`:
- Around line 1131-1147: In buildOneOfMessage on RPCCompiler, add debug/error
logging when the function currently silently returns an empty message: log when
rpcMessage.FragmentFields[typeName] is missing (include typeName and rpcMessage
identity) and when findOneOfFieldDescriptor(...) returns nil (include typeName
and rpcMessage.OneOfType.FieldName()); ensure logs give enough context to trace
schema/resolution issues (use existing logger/ctx in this package) before
returning the empty message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8616a130-9400-4c10-b055-2e22dd2d868a
📒 Files selected for processing (1)
v2/pkg/engine/datasource/grpc_datasource/compiler.go
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.266](v2.0.0-rc.265...v2.0.0-rc.266) (2026-03-26) ### Features * add support for composite types in go-tools ([#1448](#1448)) ([646ea97](646ea97)) ### Bug Fixes * preserve nullable nested input presence in gRPC compiler ([#1447](#1447)) ([900d450](900d450)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for composite types in go-tools. * **Bug Fixes** * Fixed handling of nullable nested input presence in gRPC compiler. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR adds support for composite types in the selection set of a field with a
@requiresdirective.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Checklist